Skip to content

Proposal to generate UUIDs using RandomNumberGenerators #1271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Proposal to generate UUIDs using RandomNumberGenerators #1271

wants to merge 5 commits into from

Conversation

FranzBusch
Copy link
Member

This PR adds a proposal to generate UUID's using RandomNumberGenerators

@FranzBusch FranzBusch force-pushed the fb-random-uuid branch 2 times, most recently from fb5c82f to df12e88 Compare April 24, 2025 18:59
itingliu
itingliu reviewed Apr 25, 2025
View reviewed changes
Copy link
Contributor

@itingliu itingliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need two proposal files

@FranzBusch FranzBusch force-pushed the fb-random-uuid branch 2 times, most recently from 8f18f07 to d9ffe2f Compare April 29, 2025 19:46
stephentyrone
stephentyrone reviewed Apr 29, 2025
View reviewed changes
Sources/FoundationEssentials/UUID.swift Outdated
var secondBits = second

// Set the version to 4 (0100 in binary)
firstBits &= 0xFFFFFFFFFFFF0FFF // Clear the last 12 bits
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the code wrong or the comment wrong? This appears to clear bits 12 ... 15.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment was wrong. This clears bits 48-51 of the UInt64 aligned with the RFC. Example below

10010100 10010010 10111010 11000100 11110011 01010011 00101001 11100111
10010100 10010010 10111010 11000100 11110011 01010011 00001001 11100111

@itingliu
Copy link
Contributor

@swift-ci please test

@itingliu itingliu requested a review from jmschonfeld July 28, 2025 21:42
@FranzBusch
Copy link
Member Author

@swift-ci please test

@FranzBusch
Copy link
Member Author

@itingliu I rebased the PR against latest main and migrated the tests to swift-testing

@itingliu
Copy link
Contributor

@swift-ci please test

@itingliu
Copy link
Contributor

I am very sorry that I let this implementation slip and did not get to this in time for 6.2. We'll have to defer it to 6.3 since we won't be able to merge it at this time.

@FranzBusch
Copy link
Member Author

I am very sorry that I let this implementation slip and did not get to this in time for 6.2. We'll have to defer it to 6.3 since we won't be able to merge it at this time.

No worries. The PR is targeting main and I don't have a need to get it cherry picked.

jmschonfeld
jmschonfeld approved these changes Jul 29, 2025
View reviewed changes
Copy link
Contributor

@jmschonfeld jmschonfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation LGTM aside from the availability update!

itingliu
itingliu approved these changes Jul 29, 2025
View reviewed changes
@FranzBusch
Copy link
Member Author

@jmschonfeld It looks like 6.3 is not yet defined do you want me to do this or are we landing this in a separate PR?

@jmschonfeld
Copy link
Contributor

@jmschonfeld It looks like 6.3 is not yet defined do you want me to do this or are we landing this in a separate PR?

That's almost landed in #1454 - just waiting on another check or two to finish and then will merge that. Sorry for the delay on that!

@FranzBusch @itingliu
Update availability
7e987cf 
Co-authored-by: Tina L <49205802+itingliu@users.noreply.github.com>
@FranzBusch
Copy link
Member Author

@jmschonfeld I pulled in just the Package.swift changes here as well. Feel free to merge this PR once yours landed or give me a ping.

@jmschonfeld
Copy link
Contributor

@swift-ci please test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephentyrone stephentyrone stephentyrone left review comments

@jmschonfeld jmschonfeld jmschonfeld approved these changes

@itingliu itingliu itingliu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@FranzBusch @itingliu @jmschonfeld @stephentyrone